IXWebSocketTransport::setReadyState(): Run under lock#540
IXWebSocketTransport::setReadyState(): Run under lock#540bsergean merged 1 commit intomachinezone:masterfrom awelzel:topic/awelzel/fix-set-ready-state-race
Conversation
|
Can you make the getReadyState use the mutex as well, and stop using an atomic ? Having a mutex AND an atomic seems weird. |
Thanks for the feedback. Done and re-pushed. |
|
The change looks simple, but all the thread sanitizer checks are not red .... |
|
Do you have access to a mac machine ? |
|
It's failing on Linux, too. Sorry, it's due to unprotected
Looking at the new diff, going back might be an option IMO and not all that weird: The mutex prevents concurrent execution of |
|
Alright then ... let's go back to your original idea, but do you mind adding a small comment explaining why we're doing things that way ? |
When setReadyState(CLOSED) is executed from two different threads (the server's thread, detecting a close trying to receive and a separate sending thread), there is a race where both see _readyState as non-CLOSED and both execute the _onCloseCallback(). Worse, the server's thread might be returning from ->run(), unsetting the callback while the other thread is still about to call the _onCloseCallback(), resulting in a crash rather than "just" a duplicate invocation of the callback. This change ensures that setReadyState() *and* the_onCloseCallback() are executed by a single thread till completion, by introducing a new _setReadyStateMutex instance held during setReadyState() execution.
Done. Feel free to word-smith during/after the merge. |
|
Hello @bsergean - friendly ping whether you'd consider merging this, or would rather have something changed with this PR? We're currently using a fork of IXWebSocket but would prefer to switch to your repo again :-) |
|
Thanks I did run the PR checks one last time. They don't run automatically for some reason. |
2 participants
When setReadyState(CLOSED) is executed from two different threads (the server's thread, detecting a close trying to receive and a separate sending thread), there is a race where both see _readyState as non-CLOSED and both execute the _onCloseCallback(). Worse, the server's thread might be returning from ->run(), unsetting the callback, while the other thread is still about to call the _onCloseCallback(), resulting in a crash rather than "just" a duplicate invocation.
This change ensures that setReadyState() and the_onCloseCallback() are executed by a single thread till completion.
I was tempted to remove the std::atomic<> for _readyState, but left it assuming it's still good having for getReadyState().
I've first tried _readyState.exchange() which seemed promising, but still crashed once in a while as racing threads do not wait for the _onCloseCallback() to complete.
Closes #539